-
Notifications
You must be signed in to change notification settings - Fork 250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(debugging): allow passing node args to the test runner #2609
Conversation
Add the `--testRunnerNodeArgs` option. With this option, you can pass arguments to the node process that hosts the test runner. For example, running stryker with `--timeoutMS 9999999 --concurrency 1 --testRunnerNodeArgs --inspect-brk` will allow you to debug the tests running in the dryRun or the mutation test run. Another use case is using `--testRunnerNodeArgs --cpu-prof`, this allows you to analyse the performance of the child process..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to gracefully exit the child processes as I did here: https://github.com/stryker-mutator/stryker/pull/2536/files#diff-9bd013c2c0d5a7d2bcd8594a6bfb8b4a89b54b3db0d4626b61637f00fd37900d
Without it, things that trigger on process.exit
will not trigger. The result of this is that, for example, --cpu-prof
does not output anything.
@@ -400,6 +400,14 @@ | |||
"type": "string", | |||
"default": "command" | |||
}, | |||
"testRunnerNodeArgs": { | |||
"description": "Configure arguments to be passed as exec arguments to the test runner child process. For example, running Stryker with `--timeoutMS 9999999 --concurrency 1 --testRunnerNodeArgs --inspect-brk` will allow you to debug the test runner child process. See `execArgv` of [`child_process.fork`](https://nodejs.org/api/child_process.html#child_process_child_process_fork_modulepath_args_options)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably a good idea to support and communicate with quotes around the test runner args:
--timeoutMS 9999999 --concurrency 1 --testRunnerNodeArgs "--inspect-brk"
This allows us to send multiple args like this:
--timeoutMS 9999999 --concurrency 1 --testRunnerNodeArgs "--inspect-brk --foo --bar --baz"
It also makes it clearer which args are for the main process and which for the test runner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter results in process.argv
containing:
[
'--timeoutMS',
'9999999',
'--concurrency',
'1',
'--testRunnerNodeArgs',
'--inspect-brk --foo --bar --baz'
]
So commander
should be able to handle it without issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter results in
process.argv
containing:[ '--timeoutMS', '9999999', '--concurrency', '1', '--testRunnerNodeArgs', '--inspect-brk --foo --bar --baz' ]
So
commander
should be able to handle it without issues.
Well. We currently support splitting on "," as you can see here:
I've added an example with 2 arguments in the readme: --testRunnerNodeArgs --inspect-brk,--cpu-prof
. Would this be acceptable?
Don't forget to gracefully exit the child processes as I did here: https://github.com/stryker-mutator/stryker/pull/2536/files#diff-9bd013c2c0d5a7d2bcd8594a6bfb8b4a89b54b3db0d4626b61637f00fd37900d
Without it, things that trigger on
process.exit
will not trigger. The result of this is that, for example,--cpu-prof
does not output anything.
Hmm, interesting. I don't seem to get CPU profile files at all this way. Even with the finally(() => process.exit());
. I'm using Linux, this kind of behavior will definitly be influenced by OS specific closing details. Needs further investigating. I'm hesitant to drag it into this PR, since it is not directly related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Splitting on ','
would also work, but splitting on /\s+/
(= any whitespace of any length) would make it feel a lot more like a command to me. If I didn't read the docs I would try --testRunnerNodeArgs "--foo --bar"
before trying --testRunnerNodeArgs --foo,--bar
. Both options will work for me though :)
this kind of behavior will definitly be influenced by OS specific closing details. Needs further investigating.
If you add this to the PR I'll be able to test it on Windows and MacOS. With you testing on Linux we can cover all bases. This might be out of scope for this PR though, we can also cover these bases separately. Also note that --cpu-prof
is experimental at this time: https://nodejs.org/api/cli.html#cli_cpu_prof. There might be stuff that doesn't work on every OS because of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "problem" with splitting on whitespace characters is that there are exotic cases of command-line arguments that require spaces. Not sure about node args though.
I've chosen ,
to split, because we're already using that for other options, like --mutate
or --files
(spaces in file names are a bit more common). I could change that for --testRunnerNodeArgs
to also accept a space. That might make sense as you point out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're right. There are some exotic cases that can pop up. This is generally the moment where I wonder if I can fix all of this with NPM :)
How about using something like string-argv
or spawn-args
to prevent the pain of edge cases?
Edit: I got this result by using the code in the spoiler:
Code
#!/usr/bin/env node
const commander = require('commander');
const {parseArgsStringToArgv} = require('string-argv');
function parseArgs(value, previous) {
return parseArgsStringToArgv(value.replace(/,/g, ' '));
}
const program = new commander.Command();
program.option('--testRunnerNodeArgs <args>', 'args', parseArgs, []);
program.parse(process.argv);
console.log(program.testRunnerNodeArgs);
--testRunnerNodeArgs "--foo --bar --baz \"hello world\" -h,-e,-y"
[ '--foo', '--bar', '--baz', 'hello world', '-h', '-e', '-y' ]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've chosen to split on " "
(space) for now. I've updated the docs and code. It's a better user experience than splitting on a comma.
Don't want to add an extra dependency if I don't have to. I think splitting on spaces for node-args is fine (don't see a direct reason to support escaped spaces)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for helping this feature!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem :)
Question. How could we support the command test runner here? Maybe we shouldn't allow it? At least log a warning would be appropriate I guess. |
If the command runner doesn't use child processes it's kind of moot to support it. Besides, the command runner supports passing arguments by definition. I can see testing/debugging situations where you might temporarily switch to the command runner. So I would log it at warn for those reasons. |
I've implemented the warning.
|
Add the
--testRunnerNodeArgs
option. With this option, you can pass arguments to the node process that hosts the test runner.For example, running stryker with
--timeoutMS 9999999 --concurrency 1 --testRunnerNodeArgs --inspect-brk
will allow you to debug the tests running in the dry run or the mutation test run.Another use case is using
--testRunnerNodeArgs --cpu-prof
, this allows you to analyse the performance of the child process..Fixes #2505